Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

writeCollection: Wait until previous invocation finished #157

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

adlr
Copy link

@adlr adlr commented Nov 10, 2024

Hi, Please take a look at the commit, "DRAFT: writeCollection: Wait until previous invocation finished"

I would appreciate feedback and would be happy to take another pass based on any feedback you give.

This fixes #158 for me.

Thanks very much!

Andrew de los Reyes added 3 commits November 10, 2024 13:34
This commit changes how writeCollection works. Previously, it seemed
to be a forced delay of some number of milliseconds that would need to
happen before a call to ddcutil to change brightness could occur.

Update to use a simpler logic: No more delays. If a ddcutil set
brightness command is running, queue the next command that's generated
and run it immediately after the first command completes. If a command
is queued and another one comes in, the newest one replaces the queued
one, becoming the only queued one.

This patch is a draft. If this concept is acceptable, a futher update
would be needed to remove the concept of writeCollectorWaitMs.
Alternatively, we could keep the concept and mandate a minimum
interval between two invocationsn of ddcutil, either measured from
start time to start time, or end time to start time.
@adlr adlr marked this pull request as ready for review November 10, 2024 21:50
@adlr
Copy link
Author

adlr commented Nov 10, 2024

Sorry if it's not clear. This is a draft and not ready to merge just yet. Feedback appreciated. Thanks!

@daitj
Copy link
Owner

daitj commented Nov 10, 2024

Have you tried tweaking your ddcutil options with Sleep multiplier and Queue ms, which are available from this extension's settings?

@adlr
Copy link
Author

adlr commented Nov 10, 2024

Have you tried tweaking your ddcutil options with Sleep multiplier and Queue ms, which are available from this extension's settings?

Hey, I just did on your recommendation. Looks like I need to set it 250ms or higher and indeed my problems are solved, except for a minor annoyance.

The minor annoyance is that the current code will keep a 250ms delay (in my case) before any adjustments, even if it hasn't run ddcutil in a while. It seems like it would be nice to not need that delay.

I also don't know about others setups, but if ensuring one invocation completes before the next begins means that users don't need to worry about Queue ms in the settings, that seems like a win.

I'm happy to take a pass at optimizing here, but let me know your opinion on the above before I spend too many cycles on it.

Thanks very much!

@daitj
Copy link
Owner

daitj commented Nov 26, 2024

@adlr Could you test #108 PR, it was also made because of similar issues, if that works then I should look into both of your contribution and figure out which one is best?

@adlr
Copy link
Author

adlr commented Nov 26, 2024

I just tried it, no change in behavior. I think these are addressing different issues, though. #108 PR seems to be a change in how displays are detected. My proposed change is only related to updating brightness, esp multiple changes in rapid succession (e.g. from hitting the brightness up button multiple times).

@daitj
Copy link
Owner

daitj commented Dec 5, 2024

Thanks, yeah I read it wrong. I will test it at somepoint and merge it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Display flickers in brightness when adjusting brightness
2 participants